Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

canlogserver: Close accsocket and can when tcp client disconnected #149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qianfan-Zhao
Copy link

Close all can socket and send "FYI" to tcp client when the tcp client
disconnected.

Signed-off-by: qianfan Zhao [email protected]

Close all can socket and send "FYI" to tcp client when the tcp client
disconnected.

Signed-off-by: qianfan Zhao <[email protected]>
@hartkopp
Copy link
Member

hartkopp commented Aug 6, 2019

What problem do you want to solve?
The accept() leads to a fork() and when the child is terminated due to the client termination the child is completely removed (which closes all its sockets, e.g. CAN_RAW).
I just don't see the reason for the PR ...

@qianfan-Zhao
Copy link
Author

qianfan-Zhao commented Aug 7, 2019

@hartkopp Hi, please try this patch:

do_not_fork.patch.txt

Open canlogserver and 'telnet' it in another computer, disconnect tcp link in the client right now.
Wireshark capture those step:

(canlogserver IP: 10.11.12.13, tcp client IP: 10.11.12.1)

1	0.000000000	10.11.12.1	10.11.12.13	TCP	74	52816 → 28700 [SYN] Seq=0 Win=29200 Len=0 MSS=1460 SACK_PERM=1 TSval=4168349364 TSecr=0 WS=128
2	0.000370615	10.11.12.13	10.11.12.1	TCP	74	28700 → 52816 [SYN, ACK] Seq=0 Ack=1 Win=14480 Len=0 MSS=1460 SACK_PERM=1 TSval=397751 TSecr=4168349364 WS=8
3	0.000026419	10.11.12.1	10.11.12.13	TCP	66	52816 → 28700 [ACK] Seq=1 Ack=1 Win=29312 Len=0 TSval=4168349364 TSecr=397751
8	1.268289006	10.11.12.1	10.11.12.13	TCP	66	52816 → 28700 [FIN, ACK] Seq=1 Ack=1 Win=29312 Len=0 TSval=4168355867 TSecr=397751
9	0.005321842	10.11.12.13	10.11.12.1	TCP	66	28700 → 52816 [ACK] Seq=1 Ack=2 Win=14480 Len=0 TSval=398402 TSecr=4168355867

canlogserver doesn't close the tcp link when the client disconnected. (There are no FYI sent from server).

If the can interface got a frame after the tcp client disconnected, canlogserver try write this frame to tcp client. It will got a RST.

13	75.420245719	10.11.12.13	10.11.12.1	TCP	111	28700 → 52816 [PSH, ACK] Seq=1 Ack=1 Win=1810 Len=45 TSval=438966 TSecr=4168355867
14	0.000053205	10.11.12.1	10.11.12.13	TCP	54	52816 → 28700 [RST] Seq=1 Win=0 Len=0

But canlogserver doesn't know the client disconnected, it think this can frame was sent successfully.

# ./canlogserver can0
accepted
Write 45 bytes

The next time when it try sent another can frame, it will got signal PIPE and died due to without signal handler.

# ./canlogserver can0
accepted
Write 45 bytes
shutdown_gra: got signal 13

The tcp logic seems not good.

@yashi
Copy link
Contributor

yashi commented Apr 7, 2023

Hi,

I've been reading canlogserver.c thease days. And indeed it does some odd thing.

The code structure is basicall,

  1. wait on accept()
  2. when new connection arrived, fork
    • if parent, accept again and blocked
    • if child, break out the loop and keep going
  3. open given can interfaces
  4. while running, serve a client
  5. When client closes the connection, the child get SIGPIPE and die
  6. When SIGTERM, app shuts down

When a new client connects, accept() returns, and the parent has a chance to reap SIGPIPED children. It forks and goes back to accept().

For new child, enumerate the list of can interfaces and open them again.

I'd propose

  • Close sockets and exit when write() failed, instead of SIGPIPED. That means
    • Ignore SIGPIPE
    • handle error return from write() (or send())
    • get out of the while (running)block by running = false, which doesn't end the server but just the child.

Optionally,

  • Open can interfaces before fork() so that all childen keep the same fds and no need to open.
    • assuming that multiple read() (or recvfrom()) from the fds gives you the exact copy of the captured copy, as does the current code do.

The first part goes something like:

diff --git a/canlogserver.c b/canlogserver.c
index 3fe1ad9..6e7b137 100644
--- a/canlogserver.c
+++ b/canlogserver.c
@@ -424,9 +424,10 @@ int main(int argc, char **argv)
                                sprint_canframe(temp+strlen(temp), &frame, 0, maxdlen); 
                                strcat(temp, "\n");
 
-                               if (write(accsocket, temp, strlen(temp)) < 0) {
-                                       perror("writeaccsock");
-                                       return 1;
+                               if (send(accsocket, temp, strlen(temp), MSG_NOSIGNAL) < 0) {
+                                       //perror("writeaccsock");
+                                       running = 0;
+                                       break;
                                }
                    
 #if 0

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants